Skip to content

Improve shielded sync - #4785

Merged
mergify[bot] merged 43 commits into
mainfrom
tiago/bridge-tree-shielded-wallet
Oct 2, 2025
Merged

Improve shielded sync#4785
mergify[bot] merged 43 commits into
mainfrom
tiago/bridge-tree-shielded-wallet

Conversation

@sug0

@sug0 sug0 commented Aug 7, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Describe your changes

  • Build commitment tree and witness map locally
    • Uses leanbridgetree crate for this purpose
    • If the number of blocks to sync exceeds a certain threshold, we race with the local approach, by fetching the commitment tree, witness map and notes index in the background
      • Whatever finishes first is the data we store in the shielded wallet (which should have been identical, regardless of being built locally or fetched from the MASP indexer)
  • Fix block index not being properly used
    • This is the major source of the speed-up in this PR
  • Optimize the layout of the shielded wallet
    • Removed unnecessary fields
  • Only fetch block index after a threshold of $N$ blocks (where it makes sense latency wise to wait for that network query vs. fetching $N$ potentially empty blocks)

Checklist before merging

  • If this PR has some consensus breaking changes, I added the corresponding breaking:: labels
    • This will require 2 reviewers to approve the changes
  • If this PR requires changes to the docs or specs, a corresponding PR is opened in the namada-docs repo
    • Relevant PR if applies:
  • If this PR affects services such as namada-indexer or namada-masp-indexer, a corresponding PR is opened in that repo
    • Relevant PR if applies:

@sug0 sug0 added client MASP SDK breaking:client Namada client breaking change breaking:SDK SDK breaking change wallet labels Aug 7, 2025
@sug0

sug0 commented Aug 7, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

this is currently crashing when calling remove_mark in integration tests

@sug0
sug0 force-pushed the tiago/bridge-tree-shielded-wallet branch 6 times, most recently from f1fe9d7 to ba6e668 Compare September 1, 2025 11:11
@sug0
sug0 force-pushed the tiago/bridge-tree-shielded-wallet branch 2 times, most recently from ef61aea to a52bdfb Compare September 6, 2025 11:40
@sug0
sug0 marked this pull request as ready for review September 6, 2025 11:57
sug0 added a commit that referenced this pull request Sep 6, 2025
@sug0
Changelog for #4785
f356465
@github-actions github-actions Bot added the breaking:api public API breaking change label Sep 6, 2025
@mergify

mergify Bot commented Sep 6, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

🧪 CI Insights

Here's what we observed from your CI run for f97031a.

✅ Passed Jobs With Interesting Signals

Pipeline Job Signal Health on main Retries 🔍 CI Insights 📄 Logs
Lint 🧹 Build 🛠️ Test 🚦 test-integration Base branch is broken, but retries were needed. Could be early signs of flakiness 👀 Broken 1 View View

batconjurer
batconjurer suggested changes Sep 8, 2025
View reviewed changes
Comment thread crates/shielded_token/src/masp/shielded_sync/utils.rs
Comment thread crates/sdk/src/masp/utilities.rs Outdated
Comment thread crates/shielded_token/src/masp/shielded_sync/dispatcher.rs
Comment thread crates/shielded_token/src/masp/wallet_migrations.rs Outdated
@sug0
sug0 requested a review from batconjurer September 8, 2025 09:44
sug0 added a commit that referenced this pull request Sep 8, 2025
@sug0
Changelog for #4785
de010c2
@sug0
sug0 force-pushed the tiago/bridge-tree-shielded-wallet branch from f356465 to de010c2 Compare September 8, 2025 09:44
@sug0 sug0 added the do-not-merge Do not merge for now label Sep 8, 2025
@sug0 sug0 mentioned this pull request Sep 8, 2025
Merged
sug0 added a commit that referenced this pull request Sep 29, 2025
@sug0
Changelog for #4785
1f8483f
@sug0
sug0 force-pushed the tiago/bridge-tree-shielded-wallet branch from 814598a to 1f8483f Compare September 29, 2025 09:46
grarco
grarco reviewed Sep 30, 2025
View reviewed changes
Comment thread crates/shielded_token/src/masp/shielded_sync/utils.rs Outdated
grarco
grarco reviewed Sep 30, 2025
View reviewed changes
Comment thread crates/shielded_token/src/masp/shielded_sync/dispatcher.rs
@sug0
sug0 force-pushed the tiago/bridge-tree-shielded-wallet branch from 1f8483f to f97031a Compare October 1, 2025 09:57
@sug0 sug0 added merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass backport-201.0 Backport to app 201.0 maintenance branch and removed do-not-merge Do not merge for now labels Oct 1, 2025
@codecov

codecov Bot commented Oct 1, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

This was referenced Oct 1, 2025
Closed
Closed
@mergify

mergify Bot commented Oct 1, 2025

Copy link
Copy Markdown
Contributor

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

You can check the last failing draft PR here: #4888.

You may have to fix your CI before adding the pull request to the queue again.
If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

@mergify

mergify Bot commented Oct 1, 2025

Copy link
Copy Markdown
Contributor

Hey @sug0, your pull request has been dequeued due to the following reason: CHECKS_FAILED.
Sorry about that, but you can requeue the PR by using @mergifyio requeue if you think this was a mistake.

@sug0

sug0 commented Oct 2, 2025

Copy link
Copy Markdown
Collaborator Author

@Mergifyio requeue

@mergify

mergify Bot commented Oct 2, 2025

Copy link
Copy Markdown
Contributor

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

This was referenced Oct 2, 2025
Closed
Closed
@sug0 sug0 mentioned this pull request Oct 8, 2025
Merged
@mergify mergify Bot mentioned this pull request Oct 9, 2025
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@murisi murisi Awaiting requested review from murisi
@mateuszjasiuk mateuszjasiuk Awaiting requested review from mateuszjasiuk
2 more reviewers
@grarco grarco grarco approved these changes
@batconjurer batconjurer batconjurer approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

backport-201.0 Backport to app 201.0 maintenance branch backport-libs-0.251 Backport libraries to 0.251 maintenance branch breaking:api public API breaking change breaking:client Namada client breaking change breaking:SDK SDK breaking change client MASP merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass SDK wallet

Projects

None yet

Milestone

Next Release

Development

Successfully merging this pull request may close these issues.

5 participants

@sug0 @brentstone @batconjurer @grarco @tzemanovic